Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom

FIX: Verify first word of _cmd in dependency check #1236

Merged
merged 1 commit into from
Aug 6, 2018
Merged

FIX: Verify first word of _cmd in dependency check #1236

merged 1 commit into from
Aug 6, 2018

Conversation

effigies
Copy link
Member

@effigies effigies commented Aug 6, 2018 •
edited
Loading

Changes proposed in this pull request

In the event that an interface._cmd has spaces in it - i.e., pre-set flags in addition to the command name - the which check will fail. This PR uses _cmd.split()[0] to use only the command up to the first space when checking for the command.

A command of the form python /some/path/some_script will merely check the existence of python, and not /some/path/some_script, but that would not be correctly handled by the existing check. To my knowledge, we do not have any such nodes at this time, so this is theoretical, but we may want to consider a more robust long-term solution, but I don't believe that should hold up this fix.

Fixes #1235.

Documentation that should be reviewed

None.

Pull-request guidelines:

  • I have read the guidelines for contributions.
  • I understand that my contributions will not be merged unless the work is
    finished (i.e. no [WIP] tag remains in the title of my PR) and tests pass.
  • The proposed code follows the
    coding style,
    to the extent I understood them (and I will address any comments raised by the PR's reviewers in this regard).
  • (Optional) I opt-out from being listed in the .zenodo.json file.

@effigies
Copy link
Member Author

effigies commented Aug 6, 2018

@mgxd Are you able to check build a Singularity image off of this branch, prior to release? It would be nice to have a check.

The issue should arise in Docker, as well, given the same inputs. The error criterion, I believe, should be whether a QwarpPlusMinus interface is instantiated in our workflow, and has nothing to do with Singularity.

@oesteban
Copy link
Member

oesteban commented Aug 6, 2018

I feel pretty confident this will fix the issue. Let's go ahead. @mgxd please reopen if I'm wrong. Please, let us know that it worked otherwise.

@oesteban oesteban merged commit 55fef77 into master Aug 6, 2018
@mgxd mgxd mentioned this pull request Aug 6, 2018
Merged
1 task
@mgxd
Copy link
Collaborator

mgxd commented Aug 6, 2018

@effigies @oesteban this is a good short-term solution, but should probably be reverted once the relevant interfaces are fixed and released upstream.

One case that comes to mind is wb_command <command>, in which the could vary between workbench versions. This way of checking will always assume the command works (as long as wb_command is found in the PATH)

@effigies effigies deleted the fix/dependency_check branch August 6, 2018 18:48
@effigies
Copy link
Member Author

effigies commented Aug 6, 2018

Reverting would break wb_command <command> even further. We don't pretend to check whether the arguments will work, just whether we can hit the executable.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers
No reviews
Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

Singularity build missing dependencies (1.1.3)
3 participants
@effigies @oesteban @mgxd